Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transitions with Plotly.react #3217

Merged
merged 23 commits into from
Jan 22, 2019
Merged

Transitions with Plotly.react #3217

merged 23 commits into from
Jan 22, 2019

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Nov 5, 2018

resolves #1849 (hopefully).

Transitions (don't call them animations) are coming to Plotly.react 🎉

In this iteration, I opted for a single transition option container. For example,

var transition = {
   duration: 2000,
   easing: 'cubic-in-out'
}

var data = [{y: [1,2,1], marker: {size: 20}}]

var layout = {
  xaxis: {range: [-1, 3]},
  yaxis: {range: [0, 3]},
  transition: transition
}

Plotly.newPlot(gd, data, layout)

data[0].y = [2, 1, 2]
data[0].marker.size = 40
data[0].marker.color = 'red'
Plotly.react(gd, data, layout)

gives:

peek 2018-11-05 17-34

which appears a lot smoother in reality than in that gif.


Here are more examples (taken from https://plot.ly/javascript/animations/ but converted to using Plotly.react):


In brief to get transitions working with Plotly.react, this PR:

  • added anim: true to animatable attributes and picked them off during react diffing
  • added a version of Plots.transition that assumes that gd._fullData / gd._fullLayout is the "new" state (N.B. Plotly.animate assumes that gd._fullData / gd_fullLayout is the "old" state)
  • make that new Plots.transition figure out if redraw after completion is required from restyle / relayout flags
  • track axes with autorange:true and altered ranges during react diffing to efficiently compute "layout" transitions
  • diff fullLayout before fullData to track animatable datarevision changes

TODO:

cc @plotly/plotly_js

... which will be used to set transition options during
    Plotly.react updates.
- to (in next commit) efficiently compute axis range transitions
- potentially target axis range react update (like we currently do
  in relayout)
- temporarily add Plots.transitions2 & Cartesian.transitionAxes2
- call Plots.transition2 from Plotly.react when at one animatable
  attribute has changed AND 'layout.transition` is set by user
- 'redraw' after transition iff not all changed attributer are animatable
- handle simultaneous trace + layout updates the same way as Plotly.animate
- special handling for 'datarevision' diff'ing
@nicolaskruchten
Copy link
Member

(don't call them animations)

Yes! Super-agree :)

@chriddyp
Copy link
Member

chriddyp commented Nov 5, 2018

awesome! is there a prerelease build (on a cdn?) that I can try this out on? I recall that we were creating builds on commits now

@antoinerg
Copy link
Contributor

antoinerg commented Nov 5, 2018

@chriddyp https://17875-45646037-gh.circle-artifacts.com/0/plotly.min.js

To find the link:

  • Click on "Details" for the publish step
    screenshot from 2018-11-05 18-45-53
  • Click on "Show URLs to build files"
    screenshot from 2018-11-05 18-46-23

@chriddyp
Copy link
Member

chriddyp commented Nov 5, 2018

In Dash-land, for larger features like this, we usually create pre-releases and share them with the community for a couple of weeks to get feedback. If OK with plotly.js, I'd like to do the same with this PR. I will manage the community and I'll create a pre-release in dash-core-components from this PR.

I'm frequently surprised by what folks end up doing with abstract features like this and it's a nice, low-risk way to test the waters.

@etpinard
Copy link
Contributor Author

etpinard commented Nov 6, 2018

If OK with plotly.js

If dash users are made aware of the limitations, mainly:

plotly.js/src/plots/plots.js

Lines 2547 to 2549 in 1cb0d5f

// Here handle the exception that we refuse to animate traces and axes at the same
// time. In other words, if there's an axis transition, then set the data transition
// to instantaneous.

then yeah I'm ok with it.

@etpinard
Copy link
Contributor Author

etpinard commented Nov 8, 2018

Update from today's meeting: I'll attempt to add a flag that makes trace transitions "win out" over layout transitions.

@nicolaskruchten
Copy link
Member

If the major use of this is object-constancy (as I believe, although I'm open to other beliefs!) then I'm pretty sure we need to provide a mechanism for the developer to identify the 'objects' that deserve 'constancy'. For example if I have traces A, B and C in one state and I transition to another state with just B and C I would expect A to vanish and B and C to stay, not A to transition to B, B to C and C to vanish, as they might if we used position as a key (is that what we're currently doing?).

Should we do this by letting the user specify uid or do we create a new transitionkey or objectid or something?

@etpinard
Copy link
Contributor Author

If the major use of this is object-constancy

Part of the examples I made ⬆️ https://codepen.io/etpinard/pen/XybjZG?editors=1010

@nicolaskruchten
Copy link
Member

Ah the ids key ok. But what about trace-to-trace constancy?

@nicolaskruchten
Copy link
Member

Here's an example: https://codepen.io/nicolaskruchten/pen/pQddZm?editors=0010

In this case I would expect the red trace to vanish and the blue trace to translate to where the red one used to be, but I don't think I can express that right now.

@etpinard
Copy link
Contributor Author

uid should be enough in this case. No need for another attribute.

@nicolaskruchten
Copy link
Member

OK, so we should evolve this PR a bit to only transition matching-uid-traces? @chriddyp you're OK with requiring uid for transitioning to work? I think we have to, otherwise we just encourage gratuitous transitions and/or miss some important ones...

@alexcjohnson
Copy link
Collaborator

Presumably this will work the same way as uirevision #3236 - if you don't provide uid, we assign one that's random at first but stays with traces of the same index. That machinery is nothing new. So internally we only need to key off uid, but to the user it looks like "uid if you provide that, index if you don't"

@nicolaskruchten
Copy link
Member

nicolaskruchten commented Nov 20, 2018 via email

@etpinard
Copy link
Contributor Author

Transitions from .animate and .react don't support trace-in / trace-out tweening at the moment.

@nicolaskruchten
Copy link
Member

(per verbal conversation: I'm not talking about tweening in/out but rather trace-constancy)

@etpinard
Copy link
Contributor Author

etpinard commented Nov 20, 2018

yeah, getting https://codepen.io/nicolaskruchten/pen/pQddZm?editors=0010 to work shouldn't be too hard. I suspect something is up in the react diffing.

https://codepen.io/etpinard/pen/yQpNLM?editors=0010 works ok when the before and after "data" have the same number of traces and the same trace ordering.

@nicolaskruchten
Copy link
Member

Just so we don't forget: animations should track uid regardless of the order of the traces in subsequent react() calls, which isn't the case right now: https://codepen.io/nicolaskruchten/pen/yQpNWE?editors=0010

- making uid anim:true and allowing uid to be numbers
  was enough to fix this case.
- should still go through calc (and hence a redraw after the transition)
  but Scatter.plot can handle this case fine.
@etpinard
Copy link
Contributor Author

Update: the trace "object-constancy" bugs are now fixed:

not much extra logic was needed (see commits 4ef048f, 61ee4e9 and 33a9530 for the details), d3's and the key function in this block here:

join = scatterLayer.selectAll('g.trace')
.data(cdscatterSorted, function(d) { return d[0].trace.uid; });

did the rest.

@nicolaskruchten
Copy link
Member

@chriddyp can you elaborate on your comment on "transition revision" before we merge this guy in?

... but make sure to count traces deletion as a "non-animatable"
    change (to make data-mismatch test still passes).
- to DRY up pan/scroll and axes transition (next commit)
  annotations/shapes/images redraw calls.
- add ax._imgIndices stash (similar to _annIndices/_shapeIndices),
  to make redrawComponents loop just once over the axis list.
- Plots.transition2 becomes Plots.transitionFromReact
- Plots.transition and Plots.transitionFromReact are now simple
  wrappers around a more general _transition() internal method
- transitionAxes2 is now merged into transitionAxes,
  the "find which axis to edit" logic in now in Plots.transition
- transitionAxes now uses Axes.redrawComponents and support xa-only
  and ya-only edits (new react+transition tests were adapted for this)
- with values 'layout first' and 'traces first' which determines
  whether the figure's layout OR traces are smoothly transitions
  during Plotly.react calls that generate a data AND layout diff.
@etpinard
Copy link
Contributor Author

Update from today's meeting: I'll attempt to add a flag that makes trace transitions "win out" over layout transitions.

Done in 8ab38be via transition.ordering: 'trace first'. I'm not 100% sold on that attribute name though. Please let me know if you can think of a better name for it.

Example: https://codepen.io/etpinard/pen/jXdLgG


With that ⬆️ , I'll tag this PR as reviewable.

@nicolaskruchten
Copy link
Member

So... if I delete the "traces first" line, it seems to move and pan simultaneously, which is the ideal, right? I thought that wasn't possible somehow?

@etpinard
Copy link
Contributor Author

it seems to move and pan simultaneously, which is the ideal, right? I thought that wasn't possible somehow?

That's way layout transitions (like https://plot.ly/javascript/animations/#animating-the-layout) always do.

@etpinard
Copy link
Contributor Author

etpinard commented Jan 15, 2019

... you can't animate marker color/width during those layout transitions at the moment though.

@nicolaskruchten
Copy link
Member

Ah I see, the blue/black transition was so subtle I missed it. Adding a size transition as well makes it clearer what the tradeoff is.

@etpinard etpinard requested a review from archmoj January 16, 2019 19:48
@etpinard
Copy link
Contributor Author

@archmoj is going to review this one.

Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etpinard Great work!
Please find my questions below.
Thanks.

src/plot_api/plot_api.js Outdated Show resolved Hide resolved
src/plots/cartesian/transition_axes.js Show resolved Hide resolved
src/plots/plots.js Outdated Show resolved Hide resolved
src/plot_api/plot_api.js Show resolved Hide resolved
src/plot_api/plot_api.js Show resolved Hide resolved
@archmoj
Copy link
Contributor

archmoj commented Jan 22, 2019

Wonderful feature!
💃 🕺
🕺 💃

Kindly move forward and merge it, possibly after figuring out why one test started to fail on master:
https://circleci.com/gh/plotly/plotly.js/24314

@etpinard etpinard merged commit c34d299 into master Jan 22, 2019
@etpinard etpinard deleted the transitions-in-react branch January 22, 2019 12:38
@etpinard
Copy link
Contributor Author

Thanks for the review. I think the failing test is due to the joyplot PR and the boxpoint hover fix not getting along. I'll fix that later this morning. Nothing to worry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.animate method that can handle any type of figure diff
6 participants